[BoundsSafety] Support late parsing for counted_by in type positions
#166491
Conversation
rapidsna
requested review from
AaronBallman,
apple-fcloutier,
bwendling,
delcypher,
hnrklssn,
kees and
patrykstefanski
llvmbot
added
clang
rapidsna
added
clang:bounds-safety
|
@llvm/pr-subscribers-clang Author: Yeoul Na (rapidsna) ChangesPreviously, late parsing only supported attributes in declaration position and could not resolve references to member declarations that appeared later in the struct. Additionally, the compiler incorrectly accepted This commit adds a vector of Issue #166411 Patch is 77.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/166491.diff 23 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index fa509536bf021..98b470a525d11 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7012,6 +7012,9 @@ def err_builtin_counted_by_ref_invalid_use : Error<
"value returned by '__builtin_counted_by_ref' cannot be used in "
"%select{an array subscript|a binary}0 expression">;
+def err_counted_by_on_nested_pointer : Error<
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' attribute on nested pointer type is not allowed">;
+
let CategoryName = "ARC Semantic Issue" in {
// ARC-mode diagnostics.
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index dad8efd0f017f..8f7c921fb2b1d 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1161,10 +1161,11 @@ class Parser : public CodeCompletionHandler {
IdentifierInfo *MacroII = nullptr;
SourceLocation AttrNameLoc;
SmallVector<Decl *, 2> Decls;
+ unsigned NestedTypeLevel;
explicit LateParsedAttribute(Parser *P, IdentifierInfo &Name,
- SourceLocation Loc)
- : Self(P), AttrName(Name), AttrNameLoc(Loc) {}
+ SourceLocation Loc, unsigned Level = 0)
+ : Self(P), AttrName(Name), AttrNameLoc(Loc), NestedTypeLevel(Level) {}
void ParseLexedAttributes() override;
@@ -1889,10 +1890,12 @@ class Parser : public CodeCompletionHandler {
DeclSpec &DS, AccessSpecifier AS, DeclSpecContext DSContext,
LateParsedAttrList *LateAttrs = nullptr);
- void ParseSpecifierQualifierList(
- DeclSpec &DS, AccessSpecifier AS = AS_none,
- DeclSpecContext DSC = DeclSpecContext::DSC_normal) {
- ParseSpecifierQualifierList(DS, getImplicitTypenameContext(DSC), AS, DSC);
+ void
+ ParseSpecifierQualifierList(DeclSpec &DS, AccessSpecifier AS = AS_none,
+ DeclSpecContext DSC = DeclSpecContext::DSC_normal,
+ LateParsedAttrList *LateAttrs = nullptr) {
+ ParseSpecifierQualifierList(DS, getImplicitTypenameContext(DSC), AS, DSC,
+ LateAttrs);
}
/// ParseSpecifierQualifierList
@@ -1903,10 +1906,12 @@ class Parser : public CodeCompletionHandler {
/// [GNU] attributes specifier-qualifier-list[opt]
/// \endverbatim
///
- void ParseSpecifierQualifierList(
- DeclSpec &DS, ImplicitTypenameContext AllowImplicitTypename,
- AccessSpecifier AS = AS_none,
- DeclSpecContext DSC = DeclSpecContext::DSC_normal);
+ void
+ ParseSpecifierQualifierList(DeclSpec &DS,
+ ImplicitTypenameContext AllowImplicitTypename,
+ AccessSpecifier AS = AS_none,
+ DeclSpecContext DSC = DeclSpecContext::DSC_normal,
+ LateParsedAttrList *LateAttrs = nullptr);
/// ParseEnumSpecifier
/// \verbatim
@@ -2444,7 +2449,8 @@ class Parser : public CodeCompletionHandler {
SourceLocation ScopeLoc,
ParsedAttr::Form Form);
- void DistributeCLateParsedAttrs(Decl *Dcl, LateParsedAttrList *LateAttrs);
+ void DistributeCLateParsedAttrs(Declarator &D, Decl *Dcl,
+ LateParsedAttrList *LateAttrs);
/// Bounds attributes (e.g., counted_by):
/// \verbatim
@@ -2610,7 +2616,8 @@ class Parser : public CodeCompletionHandler {
void ParseTypeQualifierListOpt(
DeclSpec &DS, unsigned AttrReqs = AR_AllAttributesParsed,
bool AtomicOrPtrauthAllowed = true, bool IdentifierRequired = false,
- llvm::function_ref<void()> CodeCompletionHandler = {});
+ llvm::function_ref<void()> CodeCompletionHandler = {},
+ LateParsedAttrList *LateAttrs = nullptr);
/// ParseDirectDeclarator
/// \verbatim
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 43a48c92fc305..9f633dd71c3f6 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -1238,6 +1238,9 @@ struct DeclaratorChunk {
ParsedAttributesView AttrList;
+ using LateAttrListTy = SmallVector<void *, 1>;
+ LateAttrListTy LateAttrList;
+
struct PointerTypeInfo {
/// The type qualifiers: const/volatile/restrict/unaligned/atomic.
LLVM_PREFERRED_TYPE(DeclSpec::TQ)
@@ -2325,13 +2328,16 @@ class Declarator {
/// This function takes attrs by R-Value reference because it takes ownership
/// of those attributes from the parameter.
void AddTypeInfo(const DeclaratorChunk &TI, ParsedAttributes &&attrs,
- SourceLocation EndLoc) {
+ SourceLocation EndLoc,
+ const DeclaratorChunk::LateAttrListTy &LateAttrs = {}) {
DeclTypeInfo.push_back(TI);
DeclTypeInfo.back().getAttrs().prepend(attrs.begin(), attrs.end());
getAttributePool().takeAllFrom(attrs.getPool());
if (!EndLoc.isInvalid())
SetRangeEnd(EndLoc);
+
+ DeclTypeInfo.back().LateAttrList.assign(LateAttrs);
}
/// AddTypeInfo - Add a chunk to this declarator. Also extend the range to
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index c67ed99b1f49e..becadbb1a39d6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2457,8 +2457,8 @@ class Sema final : public SemaBase {
/// `counted_by_or_null` attribute.
///
/// \returns false iff semantically valid.
- bool CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
- bool OrNull);
+ bool CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, unsigned Level,
+ bool CountInBytes, bool OrNull);
/// Perform Bounds Safety Semantic checks for assigning to a `__counted_by` or
/// `__counted_by_or_null` pointer type \param LHSTy.
@@ -4198,7 +4198,8 @@ class Sema final : public SemaBase {
/// ActOnFinishDelayedAttribute - Invoked when we have finished parsing an
/// attribute for which parsing is delayed.
- void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs);
+ void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs,
+ unsigned NestedTypeLevel = 0);
/// Diagnose any unused parameters in the given sequence of
/// ParmVarDecl pointers.
@@ -5071,7 +5072,8 @@ class Sema final : public SemaBase {
void ProcessDeclAttributeList(Scope *S, Decl *D,
const ParsedAttributesView &AttrList,
const ProcessDeclAttributeOptions &Options =
- ProcessDeclAttributeOptions());
+ ProcessDeclAttributeOptions(),
+ unsigned NestedTypeLevel = 0);
/// Annotation attributes are the only attributes allowed after an access
/// specifier.
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7e4a164e34eda..514f6daf44e2f 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -1943,7 +1943,11 @@ Parser::DeclGroupPtrTy Parser::ParseSimpleDeclaration(
ParsedTemplateInfo TemplateInfo;
DeclSpecContext DSContext = getDeclSpecContextFromDeclaratorContext(Context);
- ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none, DSContext);
+ // FIXME: Why is PSoon true?
+ LateParsedAttrList BoundsSafetyLateAttrs(
+ /*PSoon=*/true, /*LateAttrParseExperimentalExtOnly=*/true);
+ ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none, DSContext,
+ &BoundsSafetyLateAttrs);
// If we had a free-standing type definition with a missing semicolon, we
// may get this far before the problem becomes obvious.
@@ -2725,12 +2729,12 @@ Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes(
void Parser::ParseSpecifierQualifierList(
DeclSpec &DS, ImplicitTypenameContext AllowImplicitTypename,
- AccessSpecifier AS, DeclSpecContext DSC) {
+ AccessSpecifier AS, DeclSpecContext DSC, LateParsedAttrList *LateAttrs) {
ParsedTemplateInfo TemplateInfo;
/// specifier-qualifier-list is a subset of declaration-specifiers. Just
/// parse declaration-specifiers and complain about extra stuff.
/// TODO: diagnose attribute-specifiers and alignment-specifiers.
- ParseDeclarationSpecifiers(DS, TemplateInfo, AS, DSC, nullptr,
+ ParseDeclarationSpecifiers(DS, TemplateInfo, AS, DSC, LateAttrs,
AllowImplicitTypename);
// Validate declspec for type-name.
@@ -3136,15 +3140,37 @@ void Parser::ParseAlignmentSpecifier(ParsedAttributes &Attrs,
}
}
-void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
+void Parser::DistributeCLateParsedAttrs(Declarator &D, Decl *Dcl,
LateParsedAttrList *LateAttrs) {
if (!LateAttrs)
return;
+ unsigned NestedLevel = 0;
+ for (unsigned i = 0; i < D.getNumTypeObjects(); ++i) {
+ DeclaratorChunk &DC = D.getTypeObject(i);
+
+ switch (DC.Kind) {
+ case DeclaratorChunk::Pointer:
+ case DeclaratorChunk::Array:
+ break;
+ default:
+ continue;
+ }
+
+ // Extract `LateParsedAttribute *` from `DeclaratorChunk`.
+ for (auto *OpaqueLA : DC.LateAttrList) {
+ auto *LA = static_cast<LateParsedAttribute *>(OpaqueLA);
+ LA->NestedTypeLevel = NestedLevel;
+ LateAttrs->push_back(LA);
+ }
+ NestedLevel++;
+ }
+
+ // Attach `Decl *` to each `LateParsedAttribute *`.
if (Dcl) {
- for (auto *LateAttr : *LateAttrs) {
- if (LateAttr->Decls.empty())
- LateAttr->addDecl(Dcl);
+ for (auto *LA : *LateAttrs) {
+ if (LA->Decls.empty())
+ LA->addDecl(Dcl);
}
}
}
@@ -3217,12 +3243,6 @@ void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName,
ArgExprs.push_back(ArgExpr.get());
Parens.consumeClose();
- ASTContext &Ctx = Actions.getASTContext();
-
- ArgExprs.push_back(IntegerLiteral::Create(
- Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), 0),
- Ctx.getSizeType(), SourceLocation()));
-
Attrs.addNew(&AttrName, SourceRange(AttrNameLoc, Parens.getCloseLocation()),
AttributeScopeInfo(), ArgExprs.data(), ArgExprs.size(), Form);
}
@@ -4706,7 +4726,8 @@ void Parser::ParseStructDeclaration(
MaybeParseCXX11Attributes(Attrs);
// Parse the common specifier-qualifiers-list piece.
- ParseSpecifierQualifierList(DS);
+ ParseSpecifierQualifierList(DS, AS_none, DeclSpecContext::DSC_normal,
+ LateFieldAttrs);
// If there are no declarators, this is a free-standing declaration
// specifier. Let the actions module cope with it.
@@ -4768,7 +4789,7 @@ void Parser::ParseStructDeclaration(
// We're done with this declarator; invoke the callback.
Decl *Field = FieldsCallback(DeclaratorInfo);
if (Field)
- DistributeCLateParsedAttrs(Field, LateFieldAttrs);
+ DistributeCLateParsedAttrs(DeclaratorInfo.D, Field, LateFieldAttrs);
// If we don't have a comma, it is either the end of the list (a ';')
// or an error, bail out.
@@ -4825,7 +4846,8 @@ void Parser::ParseLexedCAttribute(LateParsedAttribute &LA, bool EnterScope,
SourceLocation(), ParsedAttr::Form::GNU(), nullptr);
for (auto *D : LA.Decls)
- Actions.ActOnFinishDelayedAttribute(getCurScope(), D, Attrs);
+ Actions.ActOnFinishDelayedAttribute(getCurScope(), D, Attrs,
+ LA.NestedTypeLevel);
// Due to a parsing error, we either went over the cached tokens or
// there are still cached tokens left, so we skip the leftover tokens.
@@ -6124,7 +6146,8 @@ bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide,
void Parser::ParseTypeQualifierListOpt(
DeclSpec &DS, unsigned AttrReqs, bool AtomicOrPtrauthAllowed,
- bool IdentifierRequired, llvm::function_ref<void()> CodeCompletionHandler) {
+ bool IdentifierRequired, llvm::function_ref<void()> CodeCompletionHandler,
+ LateParsedAttrList *LateAttrs) {
if ((AttrReqs & AR_CXX11AttributesParsed) &&
isAllowedCXX11AttributeSpecifier()) {
ParsedAttributes Attrs(AttrFactory);
@@ -6266,7 +6289,9 @@ void Parser::ParseTypeQualifierListOpt(
// recovery is graceful.
if (AttrReqs & AR_GNUAttributesParsed ||
AttrReqs & AR_GNUAttributesParsedAndRejected) {
- ParseGNUAttributes(DS.getAttributes());
+
+ assert(!LateAttrs || LateAttrs->lateAttrParseExperimentalExtOnly());
+ ParseGNUAttributes(DS.getAttributes(), LateAttrs);
continue; // do *not* consume the next token!
}
// otherwise, FALL THROUGH!
@@ -6447,21 +6472,33 @@ void Parser::ParseDeclaratorInternal(Declarator &D,
((D.getContext() != DeclaratorContext::CXXNew)
? AR_GNUAttributesParsed
: AR_GNUAttributesParsedAndRejected);
+ LateParsedAttrList LateAttrs(/*PSoon=*/true,
+ /*LateAttrParseExperimentalExtOnly=*/true);
ParseTypeQualifierListOpt(DS, Reqs, /*AtomicOrPtrauthAllowed=*/true,
- !D.mayOmitIdentifier());
+ !D.mayOmitIdentifier(), {}, &LateAttrs);
D.ExtendWithDeclSpec(DS);
// Recursively parse the declarator.
Actions.runWithSufficientStackSpace(
D.getBeginLoc(), [&] { ParseDeclaratorInternal(D, DirectDeclParser); });
- if (Kind == tok::star)
+ if (Kind == tok::star) {
+ DeclaratorChunk::LateAttrListTy OpaqueLateAttrList;
+ if (getLangOpts().ExperimentalLateParseAttributes && !LateAttrs.empty()) {
+ if (!D.isFunctionDeclarator()) {
+ for (auto LA : LateAttrs) {
+ OpaqueLateAttrList.push_back(LA);
+ }
+ }
+ LateAttrs.clear();
+ }
// Remember that we parsed a pointer type, and remember the type-quals.
D.AddTypeInfo(DeclaratorChunk::getPointer(
DS.getTypeQualifiers(), Loc, DS.getConstSpecLoc(),
DS.getVolatileSpecLoc(), DS.getRestrictSpecLoc(),
DS.getAtomicSpecLoc(), DS.getUnalignedSpecLoc()),
- std::move(DS.getAttributes()), SourceLocation());
- else
+ std::move(DS.getAttributes()), SourceLocation(),
+ OpaqueLateAttrList);
+ } else
// Remember that we parsed a Block type, and remember the type-quals.
D.AddTypeInfo(
DeclaratorChunk::getBlockPointer(DS.getTypeQualifiers(), Loc),
diff --git a/clang/lib/Sema/SemaBoundsSafety.cpp b/clang/lib/Sema/SemaBoundsSafety.cpp
index 39ab13653f5fe..8411eec7dfb3f 100644
--- a/clang/lib/Sema/SemaBoundsSafety.cpp
+++ b/clang/lib/Sema/SemaBoundsSafety.cpp
@@ -50,8 +50,8 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
- bool OrNull) {
+bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, unsigned Level,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
unsigned Kind = getCountAttrKind(CountInBytes, OrNull);
@@ -62,6 +62,12 @@ bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
return true;
}
+ if (Level != 0) {
+ Diag(FD->getBeginLoc(), diag::err_counted_by_on_nested_pointer)
+ << Kind << FD->getSourceRange();
+ return true;
+ }
+
const auto FieldTy = FD->getType();
if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
Diag(FD->getBeginLoc(),
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index fc3aabf5741ca..85e75d4e4b91a 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16873,11 +16873,13 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, bool IsInstantiation,
/// When we finish delayed parsing of an attribute, we must attach it to the
/// relevant Decl.
void Sema::ActOnFinishDelayedAttribute(Scope *S, Decl *D,
- ParsedAttributes &Attrs) {
+ ParsedAttributes &Attrs,
+ unsigned NestedTypeLevel) {
// Always attach attributes to the underlying decl.
if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D))
D = TD->getTemplatedDecl();
- ProcessDeclAttributeList(S, D, Attrs);
+ ProcessDeclAttributeList(S, D, Attrs, ProcessDeclAttributeOptions(),
+ NestedTypeLevel);
ProcessAPINotes(D);
if (CXXMethodDecl *Method = dyn_cast_or_null<CXXMethodDecl>(D))
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index a9e7b44ac9d73..2369180713ffb 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -6546,7 +6546,8 @@ static void handleZeroCallUsedRegsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
D->addAttr(ZeroCallUsedRegsAttr::Create(S.Context, Kind, AL));
}
-static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL,
+ unsigned Level) {
auto *FD = dyn_cast<FieldDecl>(D);
assert(FD);
@@ -6577,7 +6578,7 @@ static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL) {
llvm_unreachable("unexpected counted_by family attribute");
}
- if (S.CheckCountedByAttrOnField(FD, CountExpr, CountInBytes, OrNull))
+ if (S.CheckCountedByAttrOnField(FD, CountExpr, Level, CountInBytes, OrNull))
return;
QualType CAT = S.BuildCountAttributedArrayOrPointerType(
@@ -6951,7 +6952,8 @@ static bool MustDelayAttributeArguments(const ParsedAttr &AL) {
/// silently ignore it if a GNU attribute.
static void
ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
- const Sema::ProcessDeclAttributeOptions &Options) {
+ const Sema::ProcessDeclAttributeOptions &Options,
+ unsigned NestedTypeLevel = 0) {
if (AL.isInvalid() || AL.getKind() == ParsedAttr::IgnoredAttribute)
return;
@@ -7578,7 +7580,7 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
case ParsedAttr::AT_CountedByOrNull:
case ParsedAttr::AT_SizedBy:
case ParsedAttr::AT_SizedByOrNull:
- handleCountedByAttrField(S, D, AL);
+ handleCountedByAttrField(S, D, AL, NestedTypeLevel);
break;
// Microsoft attributes:
@@ -7855,14 +7857,15 @@ static bool isKernelDecl(Decl *D) {
D->hasAttr<CUDAGlobalAttr>();
}
-void Sema::ProcessDeclAttributeList(
- Scope *S, Decl *D, const ParsedAttributesView &AttrList,
- const ProcessDeclAttributeOptions &Options) {
+void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
+ const ParsedAttributesView &AttrList,
+ const ProcessDeclAttributeOptions &Options,
+ unsigned NestedTypeLevel) {
if (AttrList.empty())
return;
for (const ParsedAttr &AL : AttrList)
- ProcessDeclAttribute(*this, S, D, AL, Options);
+ ProcessDeclAttribute(*this, S, D, AL, Options, NestedTypeLevel);
// FIXME: We should be able to handle these cases in TableGen.
// GCC accepts
@@ -8171,11 +8174,22 @@ void Sema::ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD) {
// position to the decl itself. This handles cases like:
// int *__attr__(x)** D;
// when X is a decl attribute.
+ unsigned NestedTypeLevel = 0;
for (unsigned i = 0, e = PD.getNumTypeObjects(); i != e; ++i) {
ProcessDeclAttributeList(S, D, PD.getTypeObject(i).getAttrs(),
ProcessDeclAttributeOptions()
.WithIncludeCXX11Attributes(false)
- .WithIgnoreTypeAttributes(true));
+ .WithIgnoreTypeAttributes(true),
+ NestedTypeLevel);
+
+ switch (PD.getTypeObject(i).Kind) {
+ case DeclaratorChunk::Pointer:
+ case ...
[truncated]
|
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
I like all the deleted todos :-)
clang/include/clang/Sema/DeclSpec.h
Outdated
|
|
||
| ParsedAttributesView AttrList; | ||
|
|
||
| using LateAttrListTy = SmallVector<void *, 1>; |
There was a problem hiding this comment.
void* seems unusual. Don't we have a better type to use?
There was a problem hiding this comment.
This tracks pointers to Parser::LateParsedAttribute, a member struct of class Parser defined in Parser.h.
I couldn't include Parser.h because of circular dependencies. I also looked at a few other options:
- Duplicating the type in DeclSpec.h — In downstream, we created another struct called
LateParsedAttrInfoinDeclSpec.h, but it's essentially a duplicate ofParser::LateParsedAttribute, so I don't think that's the right approach. - Forward declaring
Parser::LateParsedAttribute— Can't do it since it's a nested member struct. - Hoisting
LateParsedAttributeout of class Parser — It inherits fromParser::LateParsedDeclaration, which is inherited by multiple other member structs and referenced all over the codebase. That would require too invasive changes.
I can still create a type alias to make it more readable:
using LateAttrOpaquePtr = void*;
Would that work for you?
There was a problem hiding this comment.
I see. Forward declaring it would have been nice, but that sounds very tricky. Maybe just adding a comment explaining what void* refers to is good enough?
There was a problem hiding this comment.
Sounds good. I just added comment and a type alias which was needed for ArrayRef anyway!
llvmbot
added
clang
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Some test failures are due to a recently merged PR to allow |
rapidsna
force-pushed
the
dev/yeoulna/late-parsing-for-type-attrs
branch
2 times, most recently
from
8ef023e to
8673fd7
Compare
Previously, late parsing only supported attributes in declaration position and could not resolve references to member declarations that appeared later in the struct. Additionally, the compiler incorrectly accepted `counted_by` on nested pointer types, which should be rejected. Instead, these attributes were incorrectly attached to the outermost pointer type because they were being treated as GNU-style declaration attributes rather than type attributes. This commit adds a vector of `LateParsedAttribute *` to `DeclaratorChunk`, similar to how `ParsedAttr` is attached to `DeclaratorChunk`. Since DeclSpec doesn't see the definition of `LateParsedAttribute`, it is treated as an opaque pointer type. When the late-attribute is actually parsed, it is now correctly attached to the appropriate nested type level, enabling references to members declared later when the late-parsed attribute is in type position, and rejection of invalid nested pointer cases. Issue llvm#166411
rapidsna
force-pushed
the
dev/yeoulna/late-parsing-for-type-attrs
branch
from
8673fd7 to
7ce24ab
Compare
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
I'm out of my depth reviewing this, I don't know much about late parsed attributes. Hopefully @AaronBallman can come along and take a look.
| IdentifierInfo *MacroII = nullptr; | ||
| SourceLocation AttrNameLoc; | ||
| SmallVector<Decl *, 2> Decls; | ||
| unsigned NestedTypeLevel; |
There was a problem hiding this comment.
Can you explain what this means? This is awkwardly worded and isn't clear at all what it should represent.
There was a problem hiding this comment.
Yeah, this feels like it's leaking an implementation detail for bounds safety into something more generally used for late parsed attributes.
There was a problem hiding this comment.
This represents the pointer indirection level where the attribute applies (e.g., int *__attr(N) * would be level 1).
I agree the name could be clearer - maybe PointerIndirectionLevel?
This isn't bounds-safety specific - it's a member of struct LateParsedAttribute, so any late-parsed attribute on a type position can generally take advantage of it.
There was a problem hiding this comment.
I think it's still an implementation detail though; only bounds safety cares about the pointer indirection level and I don't think [m]any other attributes will need it.
I think what I'm asking for here is whether we should make this a bit more clear in the interface. Currently, (outside of bounds safety) LateParsedAttribute is only used with declaration attributes and so it has addDecl() as a method so you can associate the late parsed attribute with each declaration it might be applied to. I would expect types and statements to follow the same general idea where there's a higher-level coupling associated with the late parsed attribute, rather than attribute-specific information like the pointer indirection level. e.g., perhaps the a different approach is to store two QualTypes; one QualType is the full type of a type which potentially will be using an attributed type at some level and the other QualType is the type the attribute is actually written on. Then the pointer indirection level can be calculated by the bounds safety attributes rather than part of the more general interface for late parsed attributes. e.g., int * [[foo]] * [[bar]] * would have two late parsed attributes. The full QualType is the same for each and is int ***, but one LateParsedAttribute gets a QualType of int ** (for bar) and the other gets int * (for foo).
But because we're expanding the capabilities here... Do we want LateParsedDeclAttribute and LateParsedTypeAttribute with the common stuff in a base class? Do we want a discriminated union in LatestParsedAttribute so only one of the decls, type, (or eventually smt) is valid at a time? My concern with the current approach is that it's unclear which fields are actually used under what circumstances,. e.g., you can get a LatedParsedAttribute which sets PointerIndirectionLevel but is actually for a declaration attribute rather than a type attribute.
There was a problem hiding this comment.
I agree bounds safety attributes are the only type attributes that need late parsing right now. That said, any late-parsed type attribute needs to know its position in the type tree, so tracking this seems inherent to the feature—not bounds-safety specific.
IOW, even if we stored both QualTypes and walked between them like you suggested, we'd still need that position-calculating logic for any LateParsedTypeAttribute.
But because we're expanding the capabilities here... Do we want LateParsedDeclAttribute and LateParsedTypeAttribute with the common stuff in a base class? Do we want a discriminated union in LatestParsedAttribute so only one of the decls, type, (or eventually smt) is valid at a time? My concern with the current approach is that it's unclear which fields are actually used under what circumstances,. e.g., you can get a LatedParsedAttribute which sets PointerIndirectionLevel but is actually for a declaration attribute rather than a type attribute.
This makes sense to me. I can try LateParsedDeclAttribute and LateParsedTypeAttribute with the common stuff in a base class.
There was a problem hiding this comment.
IOW, even if we stored both QualTypes and walked between them like you suggested, we'd still need that position-calculating logic for any
LateParsedTypeAttribute.
I guess I'm not seeing why it's needed for any type attribute. Our existing type attributes either apply to one specific kind of type (like calling conventions which apply to function [pointer] types) or they're general so they apply to any levels of nesting (like __sptr, nullability qualifiers, etc). I don't think we have any other attributes where the nesting level matters, do we?
There was a problem hiding this comment.
Conceptually, I was thinking that type attributes are generally tied to a specific position in the type tree—like how nullability works with char * _Nonnull * _Nullable where each qualifier applies to a specific indirection level, as well as __sptr qualifiers (e.g., int * __sptr * __ptr32). I assumed annotate_type would work similarly.
So my mental model is that type attributes are generally meant to be tied to a specific nested type position as written in the source, and if a certain type attribute decides to ignore the position, that's attribute-specific behavior.
Though I guess you're saying most type attributes ignore the specific type position?
There was a problem hiding this comment.
Most attributes don't need to care about the specific type position; as we're converting the specifiers into a type, we're applying the attribute at the proper level of the type. So if the attribute only applies to an int *, then it's fine to write int * [[foo]] * because we first form the int type, then the int * type, then the int * [[foo]] attributed type, then the final type. So when we go to check for application of foo, everything is fine.
Your case is different because you want to have further restrictions on the entire type, not just the piece of the type the attribute it written on. I was thinking about this a bit last night because who needs to sleep when they can think about type attributes, and I think a potentially cleaner way to handle this is from GetFullTypeForDeclarator() (in SemaType.cpp) when we're forming the full type. At that point, we can form the full int * __counted_by(size) * type by piecing each of the types together as normal, and once we've got the full type, decide that's not valid and emit a diagnostic at that point. Do you think something like that might work?
| /// Stores pointers to `Parser::LateParsedAttribute`. We use `void*` here | ||
| /// because `LateParsedAttribute` is a nested struct of `class Parser` and | ||
| /// cannot be forward-declared. | ||
| using LateAttrOpaquePtr = void *; |
There was a problem hiding this comment.
THIS feels like a code smell, and I'm pretty uncomfortable by what is happening here.
I'm hopeful that @AaronBallman can come along and take a look at this.
There was a problem hiding this comment.
We discussed other alternatives here: #166491 (comment)
I couldn't include Parser.h because of circular dependencies. I also looked at a few other options:
- Duplicating the type in
DeclSpec.h— In downstream, we created another struct calledLateParsedAttrInfoin DeclSpec.h, but it's essentially a duplicate ofParser::LateParsedAttribute, so I don't think that's the right approach. - Forward declaring
Parser::LateParsedAttribute— Can't do it since it's a nested member struct. - Hoisting
LateParsedAttributeout ofclass Parser— It inherits fromParser::LateParsedDeclaration, which is inherited by multiple other member structs and referenced all over the codebase. That would require too invasive changes.
So using an opaque type seemed like a cleanest solution, but I'm open to other suggestions.
There was a problem hiding this comment.
Could we introduce a base class, outside of Parser, that LateParsedAttribute could inherit from so we could use that type here?
There was a problem hiding this comment.
I can do that. LateParsedAttribute already inherits from LateParsedDeclaration, so we could just hoist LateParsedDeclaration out of Parser and use it directly here. That seems cleaner than introducing yet another base class. Would you be open to that if it doesn't introduce too invasive changes?
| /// | ||
| /// \param FD The FieldDecl to apply the attribute to | ||
| /// \param E The count expression on the attribute | ||
| /// \param NestedTypeLevel The pointer indirection level where the attribute |
There was a problem hiding this comment.
Hmm... why does this have to be a certain number deep, and not just calculated/stored on that type?
There was a problem hiding this comment.
Because the attribute is late parsed after the struct is fully declared, so the field's type is already constructed. We could use placeholder types and backfill later, but then we'd have to walk through all the type indirections. Or record the type positions that need the backfill anyway. Passing the level directly seemed simpler and avoids that overhead.
| ParsedTemplateInfo TemplateInfo; | ||
| DeclSpecContext DSContext = getDeclSpecContextFromDeclaratorContext(Context); | ||
| ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none, DSContext); | ||
| // FIXME: Why is PSoon true? |
There was a problem hiding this comment.
Thats a wonderful question :)
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Not the most thorough review, I'm still trying to wrap my head around the changes a bit. I did have some questions and suggestions though.
| "%select{an array subscript|a binary}0 expression">; | ||
|
|
||
| def err_counted_by_on_nested_pointer : Error< | ||
| "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' attribute on nested pointer type is not allowed">; |
There was a problem hiding this comment.
Are we sure that "nested pointer type" is sufficiently clear?
void (*fp)(int size, int *ptr __counted_by(size));
is ptr a "nested" pointer type given that it's within the declarator for a function pointer?
Maybe "on a pointer to pointer type is not allowed"?
| /// Stores pointers to `Parser::LateParsedAttribute`. We use `void*` here | ||
| /// because `LateParsedAttribute` is a nested struct of `class Parser` and | ||
| /// cannot be forward-declared. | ||
| using LateAttrOpaquePtr = void *; |
There was a problem hiding this comment.
Could we introduce a base class, outside of Parser, that LateParsedAttribute could inherit from so we could use that type here?
| IdentifierInfo *MacroII = nullptr; | ||
| SourceLocation AttrNameLoc; | ||
| SmallVector<Decl *, 2> Decls; | ||
| unsigned NestedTypeLevel; |
There was a problem hiding this comment.
Yeah, this feels like it's leaking an implementation detail for bounds safety into something more generally used for late parsed attributes.
6 participants
Previously, late parsing only supported attributes in declaration position and could not resolve references to member declarations that appeared later in the struct. Additionally, the compiler incorrectly accepted
counted_byon nested pointer types, which should be rejected. Instead, these attributes were incorrectly attached to the outermost pointer type because they were being treated as GNU-style declaration attributes rather than type attributes.This commit adds a vector of
LateParsedAttribute *toDeclaratorChunk, similar to howParsedAttris attached toDeclaratorChunk. Since DeclSpec doesn't see the definition ofLateParsedAttribute, it is treated as an opaque pointer type. When the late-attribute is actually parsed, it is now correctly attached to the appropriate nested type level, enabling references to members declared later when the late-parsed attribute is in type position, and rejection of invalid nested pointer cases.Issue #166411